-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iree
submodule
#733
iree
submodule
#733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I also don't know why we went for the script in the first place. Maybe @Abhishek-Varma knows?
I don't know but can only speculate based on the doc comment :-
Perhaps @stellaraccident can chime in here. |
Sure but I think no one was/is taking advantage of that. Eg I added skipping torch-mlir but only last week and for that, a simpler way is for iree to make torch-mlir opt-in iree-org/iree#18409 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this reason was for doing it with sync_deps.py
either, but I can't see why not to do it this way with submodules if it works and has as much flexibility in terms of clone performance. Nice one!
abb4ef6
to
7adb0ec
Compare
0d5a546
to
ad11611
Compare
@newling @jtuyls @Abhishek-Varma can I get a re-review. I changed the clone commands/instructions in order to save people time/space (basically there's a way to do what |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at the log and I think the --depth 1
flag is not working:
Run git \
git \
-c submodule."third_party/torch-mlir".update=none \
-c submodule."third_party/stablehlo".update=none \
-c submodule."src/runtime_src/core/common/aiebu".update=none \
clone \
--recursive \
--shallow-submodules \
--depth 1 \
-b $BRANCH_NAME $REPO_ADDRESS .
shell: /usr/bin/bash -e {0}
env:
CACHE_DIR: /_work/iree-amd-aie/iree-amd-aie/.container-cache
CACHE_KEY: linux-build-test-cpp-asserts-manylinux-v[2](https://github.com/nod-ai/iree-amd-aie/actions/runs/10671927949/job/29578539280?pr=733#step:4:2)-733/merge-405
BRANCH_NAME: makslevental/iree-submodule
REPO_ADDRESS: https://github.com/nod-ai/iree-amd-aie
Cloning into '.'...
Submodule 'third_party/XRT' (https://github.com/nod-ai/XRT.git) registered for path 'third_party/XRT'
Submodule 'third_party/aie-rt' (https://github.com/Xilinx/aie-rt.git) registered for path 'third_party/aie-rt'
Submodule 'third_party/bootgen' (https://github.com/Xilinx/bootgen.git) registered for path 'third_party/bootgen'
Submodule 'third_party/iree' (https://github.com/iree-org/iree.git) registered for path 'third_party/iree'
Submodule 'third_party/mlir-air' (https://github.com/nod-ai/mlir-air.git) registered for path 'third_party/mlir-air'
Submodule 'third_party/openssl' (https://github.com/viaduck/openssl-cmake.git) registered for path 'third_party/openssl'
Cloning into '/_work/iree-amd-aie/iree-amd-aie/third_party/XRT'...
Cloning into '/_work/iree-amd-aie/iree-amd-aie/third_party/aie-rt'...
Cloning into '/_work/iree-amd-aie/iree-amd-aie/third_party/bootgen'...
Cloning into '/_work/iree-amd-aie/iree-amd-aie/third_party/iree'...
Cloning into '/_work/iree-amd-aie/iree-amd-aie/third_party/mlir-air'...
warning: --depth is ignored in local clones; use file:// instead.
done.
(https://github.com/nod-ai/iree-amd-aie/actions/runs/10671927949/job/29578539280?pr=733)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for updating the README.
Ya I saw that and that's why I added -shallow-submodules. I'm not 100% on whether this works correctly but 🤷♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yeah, not sure, it says to add |
Ya the thing is I have no clue where to add this... Anyway I'll put a TODO there and make an issue |
Co-authored-by: James Newling <[email protected]>
Co-authored-by: James Newling <[email protected]>
Co-authored-by: Jorn Tuyls <[email protected]>
ff12019
to
4d41c79
Compare
This PR checks in
iree
as a submodule instead of usingsync_deps.py
. I don't know what the original theory ofsync_deps.py
but this simplifies dev and deploy.